-
-
Notifications
You must be signed in to change notification settings - Fork 441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POTEL 40 - Init Priority settings #3674
Conversation
|
Performance metrics 🚀
|
@@ -489,6 +489,9 @@ public class SentryOptions { | |||
|
|||
private @NotNull ScopeType defaultScopeType = ScopeType.ISOLATION; | |||
|
|||
private @NotNull InitPriority initPriority = InitPriority.MEDIUM; | |||
private boolean forceInit = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the other fields in SentryOptions
maybe add a new line in between.
Also these might warrant a JavaDoc description with a short explainer what they're used for
@@ -2562,6 +2592,7 @@ public SentryOptions() { | |||
*/ | |||
private SentryOptions(final boolean empty) { | |||
if (!empty) { | |||
setInitPriority(InitPriority.LOWEST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep the default when creating new SentryOptions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was intended differently, let's remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a test here for the default value of InitPriority
:
@Test
fun `when options is initialized, InitPriority is set to MEDIUM by default`() {
assertEquals(SentryOptions().initPriority, InitPriority.MEDIUM)
}
|
||
class InitUtilTest { | ||
|
||
var previousOptions: SentryOptions? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could be private.
Also, should we re-init them for each test with @Before
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
📜 Description
Sometimes Sentry.init is called multiple times, these settings allow us to control which call to init takes priority.
💡 Motivation and Context
Support different re-init scenarios as well as init calls coming from multiple integrations (e.g. OTel Agent, Logging Integration, Spring Boot)
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps